-
-
Notifications
You must be signed in to change notification settings - Fork 439
Bug: Don't abort on potential HTTP/2 connections #3645
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
In recent Qt6 releases HTTP2 is enabled by default, this was not the case for Qt5. With this change cancelling a connection via reply->abort() breaks the protocol. We should not do that. In addition, if the protocol is broken in an error situation, we could not recover from this. Fix maplibre#3644
|
@louwers can you rerun the GCC14? It seems that this is unrelated to the change. https://github.com/maplibre/maplibre-native/actions/runs/16457498317/job/46555453962 |
|
Looks OK to me (but I do not have time to test it). Will backport it then later also to the OpenGL2 branch for now. This is maybe more a question for @louwers as you have a better overview: I guess in worst case now we load too much data? Probably I'm paranoid, but this does not sound ideal for mobile devices? |
|
What do you mean? |
|
I think we abort if one zooms or pans while loading to avoid unnecessary data transfer. This MR now unconditionally disables that. I guess on desktops it's fine, but for mobile devices on data every byte saved usually matters. |
|
I wonder if the requests that are upcomming not already removed from the list. I would agree that the current live transfer will be finished in this scenario. |
|
I was looking into this a bit more. Do you have a server where this can be reproduced, as I can not do it? Also supposedly Qt should handle HTTP/2 aborts just fine. |
Absolutely, this is mbtileserver plus haproxy. https://zonekaart.nl/style.json |
|
OK, I can now reproduce this issue both on Linux and macOS using the MapLibre test style. The issue is this seems to break a unit test. I suspect there's some issue with threading and HTTP/2. |
|
There are several fixes that will land in Qt 6.9.2 (in a few days): https://bugreports.qt.io/browse/QTBUG-132124?jql=project%20%3D%20QTBUG%20AND%20text%20~%20%22qt.network.http2.connection%22%20ORDER%20BY%20created%20DESC Let's test there if the situation improves. |
|
Would you agree even if 6.9.2 would resolve the issue, it should still be mitigated for any version before? |
In principle yes, but the fix should not break timeouts handling. Also everyone reports the issue only for Qt 6.9 when HTTP/2 is used by default (if I understand correctly it was opt-in before). |
In recent Qt6 releases HTTP2 is enabled by default, this was not the case for Qt5. With this change cancelling a connection via reply->abort() breaks the protocol. We should not do that.
In addition, if the protocol is broken in an error situation, we could not recover from this.
Fix #3644